-
-
Notifications
You must be signed in to change notification settings - Fork 142
fix(http): xsrf header validation #1616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(http): xsrf header validation #1616
Conversation
|
Hi @innocenzi , sorry for the ping but would appreciate a re-review when you've got the time. Thanks! |
|
@NeoIsRecursive sorry for the delay, does Axios/Inertia work with this PR? |
|
@innocenzi yes! |
|
I'll trust you then, haven't had time to test it but it does look like it would 👍 |
Heh, I did forget one thing and that was to test with the default cookie name (forgot I had changed that to match tempest), tempest has I don't know if that is reason enough to change it to be uppercased? Sorry for missing this... 🫣 |
|
I'm fine uppercasing it, I thought cookie names were case insensitive though? |
When doing ajax requests users (me atleast) probably expect to use the
xsrf-cookievalue to provide anX-Xsrf-tokenheader.The issue is that the cookie value is an encrypted version of the csrf uuid, so the comparison will always fail (
uuid!=encryptedUuid).I had to do an
urldecodeon the header value when I tested it in an application, which is why it is there. Should this be done on the client instead?EDIT: seems like axios does this on the client, so it might be something the client should handle.
Maybe using urlencoded base64 strings by default would be nice? I think webauthn stuff uses that to make it easier sending the data around.
I used laravel as a reference for this fix/change:
https://github.com/laravel/framework/blob/1f0bcbf0941923d7f884459a9f5fcfbd16bb8ac8/src/Illuminate/Foundation/Http/Middleware/VerifyCsrfToken.php#L153
Would this be considered a breaking change?